feat(netcdf): make the container a mapping, and add the xarray-compatible aliases - #1141
Merged
Merged
Conversation
…ible aliases `nc.variables` has been a mapping for a while, but the container itself was not: `nc["t2m"]`, `"t2m" in nc`, `len(nc)` and `list(nc)` all failed, so the first thing an xarray user types did not work. Each new member delegates to `variables`, so there is one enumeration and one refusal message, not two. - add `__getitem__`, `__contains__`, `__iter__`, `__len__`, `get`, `keys`, `values` and `items` on `NetCDF` - add the read-only aliases `data_vars`, `sizes` and `attrs` - add `coords`, built from `get_dimension_values` so the storage-order contract stays in one place - add `dtypes`, `nbytes` and `info(buf=None)` `__getitem__` raises `KeyError` where `get_variable` raises `ValueError`; the mapping protocol needs `KeyError` for `in`, `get` and `dict(nc)` to behave, so the two spellings differ deliberately. `dims` is a **mapping** of name to length, as xarray's `Dataset.dims` is, and is therefore the same object as `dimension_sizes` rather than an alias of `dimension_names`, which is a list. Aliasing a mapping name onto a list would let `nc.dims["time"]` silently return a list index. `nbytes` is computed from each variable's shape and dtype and reads no pixels, so a cube larger than memory can still be sized. It counts data variables only, where xarray's counts its coordinates too.
…rospection
Two modules. The first sweeps seven stores — a plain 4-D CF cube, a container
declaring dimensions its variables do not all use, a packed 2-D store whose
variables report a renamed `subset_y_...` axis, a container holding only
`LabeledArray`s, a grouped store with `group/var` names, a curvilinear store that
reads more names than it enumerates, and a single-variable store — and asserts that
every new member delegates totally rather than enumerating a second time.
Three things are pinned because they look like inconsistencies and will otherwise
be "fixed":
- `nc["nope"]` raises `KeyError` where `nc.get_variable("nope")` raises `ValueError`
- `nc.dims` is a mapping and is therefore not `nc.dimension_names`
- `dtypes`, `nbytes` and `info` all answer with `read_array` patched to raise, so a
`sum(v.read_array().nbytes)` implementation cannot pass
The second asserts `sorted(nc) == sorted(nc.to_xarray().data_vars)` on the 23 stores
where it holds, and gives the three where it does not a named test each: the grouped
store, where an xarray `Dataset` is one flat namespace and holds one size per
dimension name, so 8 names are flattened and 20 variables are skipped — both warned
about; and the GOES and UGRID stores, where the export carries an array CF
classification leaves out of `variable_names`. All three remain reachable through
`get_variable`.
A coverage pass showed every new body exercised, but two kinds of scenario were
missing. Both are now pinned.
A variable subset is a `NetCDF` too, so every new member is reachable on it, and
the answers follow from the members they delegate to being *container* concepts:
`variable_names` is empty, so `len`, `list`, `dict`, `dtypes` and `nbytes` all
report nothing — `nc["temperature"].nbytes` is 0 for a variable that plainly holds
2880 bytes. `dimension_sizes` needs a root group a subset does not have, so `dims`
is `{}` while `dimension_names` still reports four names from the cache built with
the subset. Each is documented on the property it surprises, because the natural
reading of `variable.nbytes` is not what it answers.
That last one settles which member `coords` iterates: `dimension_names`, not
`dims`. Keying off `dims` would throw away the axes a subset can still read, so
`set(coords) <= set(dimension_names)` is the invariant, and the test now asserts
that rather than the `dimension_sizes` version that only held on containers.
Also:
- pin the `"unknown"` arm of `_variable_dtype`, unreachable from any fixture since
no store has a zero-band variable, through a stub
- pin the `LabeledArray` arm of both sizing helpers directly
- assert `values()` and `items()` return the same objects `__getitem__` does, not
merely the same count
- assert a returned mapping or list can be mutated without reaching the container
- assert `info`'s section order and that an attribute-less container still closes
- drop the dead `types.get(name, "unknown")` fallback in `info`; it iterates the
same `variable_names` that built the mapping
…sserting equality Most examples read `nc.data_vars == nc.variable_names` -> `True`, or `nc.sizes == nc.dims == nc.dimension_sizes` -> `True`. That confirms the delegation and teaches nothing: a reader cannot see what any of them returns, and the doctest passes just as well if both sides are wrong together. Every example now prints a real value and then does something with it — the names of a five-variable store and the band counts they reach, the index a pressure level sits at, the megabytes a store would cost to read, the variables with more than one band. `info` prints its whole summary, with the tabs expanded so the example is readable. Also: - give `_variable_dtype` and `_variable_nbytes` examples; both branch on the variable kind and `_variable_dtype` has a documented `"unknown"` return, so neither is self-evident from the signature - add a second example to each member that had only one - add `See Also` to `__contains__`, `__iter__`, `__len__`, `keys`, `values`, `items` and `get`, which had none 48 doctests pass.
M1. The new `keys`/`values`/`items` promise lists, and `_LazyVariableDict` returns lists — but the `variables` property was annotated `dict[str, ...]`, so mypy resolved the delegation to `dict_keys` / `dict_values` / `dict_items` and reported three `[return-value]` errors. The mypy gate is a required check and was red on this branch. The annotation was simply wrong: the property has only ever returned a `_LazyVariableDict`, whose own list returns are already declared and already carry `type: ignore[override]` for the same reason. Naming the real type makes the three delegations check out with no cast and no second ignore. The backing `_cached_variables` slot is narrowed to match. mypy: 151 source files, no issues.
M2. `_LazyVariableDict.keys()` returned `_names` itself — the same list object on
every call, and the one `__iter__`, `__len__`, `__contains__`, `values`, `items`
and `dict(nc)` are all driven from. Mutating what `keys()` returned corrupted the
container:
k = nc.keys(); k.append("INJECTED")
list(nc) -> ['temperature', 'INJECTED']
len(nc) -> 2
dict(nc) -> ValueError: INJECTED is not a valid variable name
The leak predates this branch, but promoting it to the container's public surface
is what made it reachable as `nc.keys()`, and the docstring then said it was safe
to reorder in place. Fixed at the source so `nc.variables.keys()` is safe too.
The test written to catch this could not: it asserted `variable_names`, which
re-reads the store and so reports the right answer while the container is
corrupted. It now asserts `list(nc)`, `len(nc)`, `in` and `items()`, and a second
test covers the mapping one level down.
The `keys()` example demonstrated the unsafe idiom while passing, because
`sorted(..., reverse=True)` copies. It now reverses the returned list in place,
which is the operation that used to corrupt the container.
M6. `data_vars` returned `variable_names`, a list, so `nc.data_vars["t2m"]` raised `TypeError: list indices must be integers` and `nc.data_vars.items()` raised `AttributeError`. Only iteration and `len` happened to work on both sides. That is precisely the collision `dims`' own docstring spends fifteen lines arguing must never be introduced — "a reader coming from xarray writes `nc.dims["time"]` and would silently get a list index" — applied to the alias such a reader reaches for first. The class argued one rule and broke it a screen away. `data_vars` is now `variables`. Iterating still yields the names and `len` is unchanged, so `list(nc.data_vars)` reads as before; indexing and `.items()` start working. It still differs from xarray in what the values are — a `NetCDF` subset or a `LabeledArray` rather than a `DataArray` — and the docstring says so. This diverges from the plan's spec, which said `data_vars` -> `variable_names`, for the same reason `dims` already diverged from it on this branch. BREAKING CHANGE: `NetCDF.data_vars` returns the variables mapping rather than a list of names. `list(nc.data_vars)`, `len(nc.data_vars)` and iteration are unaffected; code comparing it directly to a list (`nc.data_vars == [...]`) or slicing it must use `nc.variable_names` instead.
…peError
M5. `_variable_dtype` documents `"unknown"` as its return for a subset reporting
no bands, and `_variable_nbytes` — its only caller — handed that straight to
`np.dtype`:
_variable_nbytes(no_bands) -> TypeError: data type 'unknown' not understood
numpy evaluates that operand whether or not the cell count is 0, so the one input
the paired helper documents would abort `nc.nbytes` and `nc.info()` instead of
contributing nothing.
The test covered only the dtype half, so the failure was invisible; it now calls
both helpers on the same stub.
M4. `open_as_multi_dimensional=False` is a documented parameter of `read_file`, and `selection.py` builds classic variables internally, but none of the three new members worked on one. Two separate failures: - `info` called `_variable_dim_names(rg, name)` with `rg = None` — classic mode has no multidim group — which reaches `None.OpenMDArray` and raises `AttributeError`. Each variable is now printed with an empty axis list, since there is genuinely nowhere to read its axes from. - On two stores the classic subdataset enumeration reports a name GDAL then declines to open (`precipitation_flux`, `O3.COLUMN.PARTIAL_AVK`), so all three members propagated a raw GDAL `RuntimeError` out of a legitimately opened container. That enumeration is the defect, and it predates this branch; these members are simply the first to iterate every name it reports. Introspection now describes what it can: an unopenable name reports `"unknown"` and contributes 0 bytes. Deliberately visible rather than skipped — `unknown` appears in both `dtypes` and the `info` summary, so the underlying enumeration problem is not hidden by the workaround. The two sizing helpers take a small `Protocol` rather than `NetCDF`, which is what they actually need and what lets the no-band test use a four-line stub. mypy: 151 source files, no issues.
M3. `__iter__` and `__len__` changed how every duck-typing site in the ecosystem
sees a container, silently:
np.asarray(nc) 0-d object array -> array of the names
np.array([nc], dtype=object).shape (1,) -> (1, 1)
isinstance(nc, Iterable/Sized/Container) False -> True
The decision was to document the new behaviour rather than add an `__array__`
that refuses the coercion. So it is written down in three places — the `__iter__`
and `__len__` docstrings, and a migration entry alongside the `data_vars` change —
and pinned by tests, including the two protocols that did *not* flip (`Mapping`,
`Sequence`) and the fact that `bool(nc)` still refuses because `Dataset.__bool__`
takes precedence over `__len__`.
The practical case is a helper that accepts "anything iterable": it used to reject
a container outright and now receives a list of name strings, so a mistake fails
later and less clearly. That is called out specifically.
L1 and L2. The guard patched `NetCDF.read_array`, which `dtypes`, `nbytes` and `info` never call — their reads go through `get_variable` -> `_read_variable` -> `MDArray.ReadAsArray`. So both "reads no pixels" tests were inert and could not fail whatever the implementation did. Counting the real call found more than the claim admitted, in both directions: - On a raster store the members read every **coordinate** axis — `lat`, `lon`, `time`, `plev`. No data variable is read, which is what "reads no pixels" legitimately means, but it is one small array per axis rather than nothing. - On a store with no raster plane they read every **data variable**: a `LabeledArray` materialises its array when built, so `nc.nbytes` on the UGRID store issues one read per variable and leaves all five in the cache. The docstrings denied this outright. The tests now assert the precise contract — the names read are disjoint from `variable_names` and are all dimensions — rather than "nothing was read", which is false and would only invite a weaker guard later. A third test pins the `LabeledArray` case that does read, so the claim and the behaviour cannot drift apart again. The docstrings say all of it, including that a string variable's `nbytes` counts object pointers and a packed variable is sized by its stored type, not by what `read_array` returns.
L3 and L4. Eight tests compared an alias to the member it literally returns — `data_vars` to `variable_names`, `sizes`/`dims` to `dimension_sizes`, `attrs` to `global_attributes`, `coords[name]` to the accessor it is built from. None can fail while the one-line delegation stands; they catch a future re-implementation and nothing else, so a member returning a wrong answer would pass every one. Each now has a companion pinning what the file declares, read with `ncdump -h`: the four dimension lengths, the single `Conventions` attribute, the CMIP store's mix of `float32` fields with an `int32` mask, and three coordinate axes — one ascending, one descending — so an orientation applied in `coords` would show up. The `dtypes` sweep asserted only that `np.dtype(...)` succeeds, which passes for the meaningless `'object'` a string variable reports. L4: `test_a_container_with_no_global_attributes_still_closes` never reached the case it names. `global_attributes` re-reads the GDAL root group on every call, so `.clear()` emptied a throwaway and the store still printed all 55 of its attributes; the test then asserted only what a neighbouring test already covers. It now patches the property and asserts the header line is followed directly by the closing brace.
…it echoes L5 and L6. The aliases exist so xarray habits transfer, which makes every place they do not transfer worth naming on the member itself. Four were undocumented: - `dtypes[name]` is a `str`, xarray's is a `numpy.dtype`, so `nc.dtypes["t2m"] == np.float64` is `False` here and `True` there — and it is `False`, not an error, so nothing signals the mismatch. - `coords[name]` is a bare `ndarray`, xarray's is a `DataArray`, so `nc.coords["lat"].values` raises `AttributeError`. - `keys()` is a snapshot, xarray's is a live `KeysView`. - `dims` / `sizes` are plain dicts where xarray's are `Frozen`, so `nc.dims["lat"] = 1` succeeds against a throwaway and is silently discarded where xarray raises. That is the one place the xarray spelling is the safer of the two, and it is pinned as a known divergence rather than as a feature. L6: a classic container's subdataset enumeration can report the same name twice — `cf__40v__1d28-2d9-3d3__nc4.nc` has 12 names and 9 distinct ones — so `len(nc)` is 12 while `len(nc.dtypes)` is 9. The mappings are keyed by unique name and `variable_names` is a list; the sweep's invariant now says so by comparing against `set(variable_names)`, and `dtypes` documents it.
L10. `docs/reference/netcdf/public-api.md` opens by claiming to map "every public member the NetCDF class itself defines — 65 in all". The class now defines 77, and none of the twelve new members appeared in any of its tables, so the page a reader scans to find a member could not lead them to one. The counts are corrected, the mapping protocol is added to the variables table, and the aliases and the introspection trio get a section each. The page said 65 and nothing noticed, because nothing compared it to the class. A test now counts the members by reflection and asserts the page's own figures, plus one assertion per new member that it is indexed at all.
…ibute lines The review's nits, in one pass since they are all small and all on the same members. - `info(buf)` is typed `TextIO | None` rather than `Any`, so a caller passing a path is a type error rather than an `AttributeError` at print time. - `coords` is annotated `dict[str, np.typing.NDArray]`, matching its own Returns block and `get_dimension_values`; it said `dict[str, Any]`. - `get` is annotated `Any` rather than `NetCDF | LabeledArray | Any`, which collapses to `Any` anyway and only read as though it meant something. The union moves into the Returns block, where it can say why. - `info` collapses newlines in an attribute value and truncates it at 120 characters. A ROMS store carries a `CPP_options` over a thousand characters and an `NLM_LBC` full of raw newlines, either of which destroys the `ncdump -h` shape the method imitates. - `__iter__` says `xarray.Dataset` where it meant xarray's, not this class's own base of the same name, which has neither member it refers to. - `__getitem__` says the protocol is read-only and points at `add_variable` / `set_variable` / `remove_variable`, since `nc["t2m"] = other` raising `TypeError` now reads like an oversight rather than a design. - The parity sweep drops its `len(DIVERGENT) == 3`, which was a second place to edit for the same change and said nothing the partition assertion did not, and states next to the constant that the glob runs at import time — so a fixture added for an unrelated feature joins this sweep.
Branch coverage of the new members was already 100% — 98/98 statements, 16/16 branches — so this goes after what coverage.py structurally does not count: ternaries, `or` fallbacks, members of an exception tuple, and comprehensions over an empty iterable, whose exit arc is the same as a non-empty one. Three gaps were real, in the sense that deleting the code left the suite green. - `_open_variable` catches `(RuntimeError, KeyError, ValueError)`, but only `RuntimeError` is reachable from a fixture — the classic enumeration naming an array GDAL will not open. The other two are the mapping's own spellings of a miss and sat in the tuple unexercised, so dropping either passed while `dtypes`/`nbytes`/`info` started raising on a container they are documented to describe. A fourth test pins that a `TypeError` still propagates, so a later widening to `except Exception` fails instead of silently swallowing everything. - On a classic container `dimension_names` is `None`, not `[]`, so `coords`' `or []` guard is the only thing standing between a documented open mode and `TypeError: 'NoneType' object is not iterable`. Nothing reached it: the classic tests exercised only `dtypes`, `nbytes` and `info`. - `_summarised` had no assertion anywhere. Its truncation ran on fixture data and nothing checked the result. Now pinned on a value whose *repr* spans lines — an ndarray, since `repr` escapes a string's newline and no string attribute reaches that arm — on the CR arm, on the ROMS store's newline-laden `NLM_LBC` yielding one printed line per attribute, and on the CMIP `history` rendering to exactly 123 characters ending in an ellipsis.
A docstring pass that ran each claim rather than reading it found six that were
simply untrue, most of them written on this branch:
- `info` said a classic container differs only in printing an empty axis list,
and that "dimensions, dtypes, attributes are unaffected". All three are wrong:
`dimension_sizes` is `{}` so the dimensions section is empty, `dtypes` prints
`unknown` for names GDAL declines to open, and `attrs` falls back to
`GetMetadata()`, which is more numerous and differently spelled
(`:NC_GLOBAL#Conventions`). A doctest now pins the real output.
- `__contains__` claimed `get_variable` reads the arrays it reports absent, with
`lat` as the example. `nc.get_variable("lat")` raises `ValueError` — a
dimension coordinate is not readable that way. Repointed at `coords`, which
does read it, and kept `get_variable` for `lat_bnds`, which it genuinely reads.
- `__getitem__` said an unknown name is "a `KeyError` naming what the store
holds", with the output elided as `KeyError: ...`. It is a bare
`KeyError: 'nope'`; only a readable non-data name gets the helpful message. The
ellipsis is what hid it, so both cases are now pinned exactly.
- `__iter__` said `np.array([nc], dtype=object).shape` becomes `(1, 1)`. It is
`(1, n)` for an n-variable store — `(1, 5)` on the CMIP fixture. The same wrong
figure was in the migration entry and is corrected there too.
- `dims` said it is "the same object as `dimension_sizes`". It is not: a fresh
dict per call, which the next sentence already relied on by calling a write a
throwaway.
- `dtypes` opened with "one entry per name in `variable_names`" and then
explained two paragraphs later that a classic store has 12 names and 9 entries.
Classic-container behaviour was also missing from `dims`, `sizes`, `coords` and
`attrs` — `coords` stated an invariant against `dimension_names`, which is `None`
there, so the documented expression was itself a `TypeError`.
Added `Raises: RuntimeError` to `get`, `__getitem__`, `values` and `items`, all of
which propagate GDAL's refusal on a classic container. `get` is the notable one:
a present name is not a guarantee of a value, and `default` does not cover it.
50 doctests pass.
…iable
H1. `__iter__` and `__len__` made NumPy treat every `NetCDF` as a sequence. On a
container that produced an array of variable *names* — visibly not data. On a
**variable**, iteration yields nothing at all, so NumPy built an empty array:
np.mean(nc["ua"]) origin/main: AttributeError branch: nan
`ua` holds 557,056 real values. `nan` is what averaging nothing gives, it looks
like a legitimate result, and it can travel a long way before anyone questions
it. The container case was documented earlier on this branch; the variable case —
the object that actually holds data — was the dangerous half and was neither
documented nor tested.
`__array__` now refuses, restoring what NumPy did before the mapping protocol
existed, and the message names `read_array()`. This is the judgement
`Dataset.__bool__` already makes for `if nc:`: there is no honest single answer,
so decline to invent one.
`dtype=object` is still honoured — boxing reads nothing and fabricates nothing,
and `np.array([nc], dtype=object)` stays `(1,)` as on main, so the refusal costs
nothing that worked before. Arithmetic, comparison, iteration, `len`, `coords`
and the introspection trio are all unaffected.
Two claims in the migration entry went with it:
- It said an object array of containers with different variable counts "is now
ragged, and NumPy raises". It does not: `np.array([nc1, nc5], dtype=object)` is
`(2,)` with no exception.
- It framed `data_vars` as a hard breaking change. `data_vars` has never existed
on `origin/main` — it was added as a list and reshaped to a mapping inside this
branch, so there is nothing downstream to migrate and the entry is removed.
M3: `nbytes` now warns when it could not size a variable. On
`cf__12v__1d4-2d5-3d2-4d1__y-asc.nc` opened classically the enumeration names the
three real cubes by `standard_name` (`precipitation_flux` for `pr`), GDAL declines
all three, and the total comes to 268,304 against a true 2,752,512 — 9.7%, and
indistinguishable from an ordinary answer. The only test of it was
`assert nc.nbytes >= 0`, which cannot fail. The warning names the skipped
variables and points at `open_as_multi_dimensional=True`; a classic store that
opens cleanly still warns about nothing.
…nto the variables mapping
M4. `_summarised` did `repr(value).replace("\n", " ")` — stripping the
two-character escape sequence, not a newline. Any value holding a backslash lost
part of itself:
C:\new\data -> C:\ ew\data
The replacement was never needed. `repr` already escapes a string's newlines, so
a string attribute is one line before it arrives; only a non-string repr, such as
an array's, can still span lines. It now collapses real control characters only,
and the ROMS store's newline-laden NLM_LBC still prints on one line — escaped, as
`ncdump -h` prints it, rather than silently rewritten. No fixture has a backslash
in an attribute, which is why the existing tests could not catch this.
M5. `nc.data_vars["X"] = ...` left the container disagreeing with itself:
nc["X"] -> the injected value
"X" in nc -> False
list(nc) -> unchanged
`_LazyVariableDict` subclasses `dict` to cache what it has loaded, while `_names`
— what `__iter__`, `__len__` and `__contains__` answer from — is a separate list,
so a write landed in the cache and nowhere else. The hole predates this branch on
`variables`; `data_vars` is the same object and inherited it, and the round-1
class asserting this invariant for the other five mappings had omitted it.
`__setitem__` and `__delitem__` now refuse, pointing at `add_variable` /
`set_variable` / `remove_variable`. The internal cache fill calls
`dict.__setitem__` directly, so the laziness is untouched — asserted by identity
on a repeated lookup.
tests/netcdf: 4087 passed, 72 skipped.
M6. The rationale for making `dims` a mapping is "as xarray's `Dataset.dims` is", and that is on its way out. The installed xarray (2026.7.0) already returns a `FrozenMappingWarningOnValuesAccess` which warns when the values are read, en route to returning a set of dimension *names* — which will make xarray's `dims` closer to `dimension_names` than to this one. The docstring, the reference page and the PR all asserted the parity with no mention of it. `dims` is not following it. A mapping is what a reader arriving today expects, and matching a set of names would bring straight back the collision the name was chosen to avoid. But `sizes` is the spelling that stays a name-to-length mapping on both sides, so both the docstring and the reference page now point at it for code meant to last.
… variables" are The round-2 should-fixes and nits. L4 — `_open_variable` caught three exception types and said nothing. The classic enumeration naming a variable by its standard_name is the case it exists for, but those exceptions have other sources: a `/vsicurl/` store raises `RuntimeError` on a transient network failure, and `get_variable` raises `ValueError` for more reasons than a bad name. Either came back as a quietly smaller `nbytes` and an `"unknown"` dtype, indistinguishable from the known wart. It now warns per name, carrying the underlying exception and both possible remedies. L5 — `info` enumerated the store twice: once inside `dtypes` and again for its own loop, then indexed the first by the second. Each walk re-queries `GetMDArrayNames()` and re-runs CF classification, and any disagreement between them was a bare `KeyError` out of a read-only summary. One snapshot now. L6 — `nbytes` summed `variable_names` while `dtypes` is keyed by name, so a classic container reporting a name twice would have been sized twice. Latent rather than active, because the one store that repeats a name repeats one GDAL refuses. Both `nbytes` and `info` now work over distinct names, so all three members agree on what "the data variables" are, and `nbytes` says so. L1/L2 — the `variables` docstring still promised `dict[str, ...]` after round 1 changed the signature to `_LazyVariableDict`, so mkdocstrings would have rendered two return types for one property. Corrected, with a note that the concrete type is internal and should be treated as a read-through mapping. L3 — `_HasDtype`'s rationale was false: it claimed structural typing was what let a test use a stub, but `[tool.mypy] exclude` skips `tests/`, so a nominal annotation permits the same stub — as `_open_variable`, stubbed identically by its own tests, demonstrates. The rationale is now the true one, and the Protocol is split so each helper's annotation states only what it reads: `_variable_dtype` touches `dtype` alone, and only `_variable_nbytes` needs the shape. L7 — the two tests that are load-bearing on unrelated work now say what to edit when they fail, and the docs-page counts match on a word boundary; `1 staticmethod` passed against a page reading `1 staticmethods`. L8 — `assert np.dtype(x) is not None` and `assert get_variable(x) is not None` cannot fail: both expressions return an object or raise. Replaced with the claims actually intended — that the dtype name round-trips through numpy unchanged, and that the array is reachable and holds data. N1 — a truncated attribute lost its closing quote, so the `:key = 'value ;` line no longer parsed as the `ncdump -h` shape `info` imitates. N3 — `info` on a grouped store declares the root group's single `recNum` and then names it on 29 variables whose lengths differ. Faithful to `dimension_sizes`, but worth saying. N4 — the new docs tables are column-aligned.
The write refusal added earlier overrode `__setitem__` and `__delitem__` and
stopped there. CPython implements the rest of `dict`'s mutators in C against the
underlying storage, so none of them route through a subclass override:
nc.variables.update({"GHOST": "not a variable"})
nc["GHOST"] -> 'not a variable'
"GHOST" in nc -> False
list(nc) -> ['temperature']
which is the exact inconsistency the refusal was added to prevent. `setdefault`,
`pop`, `popitem`, `clear` and `|=` were open the same way, and `clear()` was worse
than a no-op: it emptied the cache, after which `popitem()` reported an empty
dictionary for a container that plainly holds a variable.
All eight now raise through one shared message. The internal fill calls
`dict.__setitem__` on the class, so the laziness is untouched — asserted by
identity on a repeated lookup.
Three members were also still disagreeing about their work, having been made to
agree only about their keys:
- `dtypes` walked the raw `variable_names` while `nbytes` and `info` walked
distinct names, so a classic store reporting a name four times re-opened it four
times: 5 unopenable variables produced 8 warnings. It is now a plain loop over
distinct names, which also puts `_open_variable`'s warning back on the caller's
line — the generator frame a comprehension adds had been pointing it at this
file.
- `info` read `variable_names` again after `dtypes` had already walked it, so the
comment claiming one enumeration described a hazard the code still had: any
disagreement between the two walks was a bare `KeyError` from `types[name]`, out
of a read-only summary. The names now come from the mapping `dtypes` returned.
- `_summarised` tested the *first* character for a quote when restoring the one
truncation removes, so a `bytes` value — whose repr opens `b'` — kept losing it.
Tested on the last character now.
A second docstring pass, verifying each claim by running it rather than reading it. Three were untrue, all written within the last few commits: - `_LazyVariableDict.clear` warned that emptying the cache would leave `popitem` reporting an empty dictionary for a container that plainly holds variables. It cannot: the commit that closed the other six doors made `popitem` refuse too. Replaced with the consequence that is real — nothing goes missing, since `_names` drives `in`, `len` and iteration, but entry *identity* breaks, which is the guarantee freezing a `LabeledArray` entry exists to provide. - `__ior__` claimed its return type "matches `dict.__or__`'s so the two stay compatible for a type checker". They are incompatible — that is why the line carries a `type: ignore[misc]`, and the comment three lines above already said the mismatch was the point. The docstring contradicted its own code. - `coords` said a subset's spatial axes drop out *because* the y axis is renamed to the window it was cut with. A subset drops every spatial axis regardless: `cf__20v__1d3-3d17__y-desc.nc::tcw` renames neither `latitude` nor `longitude` and loses both. The rename is incidental; the reason is that a subset does not track its spatial axes at all, which is what `get_dimension_values` documents. Pinned with a doctest on the un-renamed store. `_open_variable`'s warning said a name "is enumerated by the store but could not be opened" on the `KeyError` branch, which is reached for a name the store never enumerated. Reworded to describe what happened without asserting why. Added the missing `Warns:` sections — `_open_variable`, `dtypes` and `info` all emit warnings and none said so — and gave `_LazyVariableDict` a class docstring that names the eight refused entry points and says why `|` is left alone. Full suite: 12,247 passed, 89 skipped. mypy clean.
SonarCloud S5778 (x2) and S9088 (x3), all in the new suites. Each block wrapped more than one call that could raise or warn, so a passing test did not establish which call produced the result — the stub construction sat inside the block alongside the call under test, and one block wrapped two `_open_variable` calls. The stubs are hoisted, the two-name case loops over one instance, and the nested `pytest.warns` in the grouped-store parity test is replaced by a single `pytest.warns` whose recorded messages are then filtered. That last one also reads better: it asserts exactly one warning of each kind rather than "at least one", which the nested form could not distinguish. Behaviour under test is unchanged; 359 tests still pass.
…lue to an error
Both the migration entry and the `__iter__` table claimed `np.asarray(nc)` "raises
exactly as it did before". Measured against a class with the new dunders removed,
it did not:
np.asarray(container) main: 0-d object array now: TypeError
np.asarray(variable) main: raises now: raises
So a container's coercion goes from a useless value to an error — a hard change,
even though the old value was a box around the object rather than any of its data.
The variable case is the one the refusal was added for and is unchanged in kind:
it raised before and raises now.
Written while the earlier claim was still fresh, which is how it got in: the
variable case was checked and the container case was assumed to match it.
|
MAfarrag
added a commit
that referenced
this pull request
Sep 15, 2026
The reflection test added in #1141 caught this: `NetCDF` now defines 78 public members rather than 77, and `isel` appeared in none of the page's tables. Worth noting that the test earned its keep on the first task after it landed — it fired from `tests/netcdf/structure/`, on a branch that touched neither that directory nor the docs, and its message named the page, the new count and what to edit. The page had drifted silently from 65 to 77 before anything checked it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Description
Tasks T1, T2 and T16 of
planning/xarray/missing-functionality-plan.md— the Tier 1 ergonomicsbatch.
nc.variableshas been a mapping for a while, but the container itself was not:nc["t2m"],"t2m" in nc,len(nc)andlist(nc)all raisedTypeError, which is the first thing a reader arriving fromxarray types. Every member added here delegates to
variables, so there is one enumeration and one refusalmessage rather than a second list that can drift.
T1 — the mapping protocol on the container.
__getitem__,__contains__,__iter__,__len__,get,keys,valuesanditems.__iter__yields data variables only, matchingvariablesandvariable_names— xarray'sDataset.variablesincludes coordinates while this class's does not, so thedocstring says which rule applies.
T2 — the xarray-compatible aliases.
data_vars,dims,sizes,attrs, and a newcoordsbuilt fromget_dimension_valuesso the storage-order contract stays in one place.T16 — cheap introspection.
dtypes,nbytesandinfo(buf=None). No data variable's array is read.Two mapping-vs-list decisions, made the same way
xarray's
Dataset.dimsandDataset.data_varsare both mappings. pyramids' nearest members —dimension_namesandvariable_names— are both lists. Aliasing a mapping name onto a list is thecollision this class is trying to avoid: a reader writes
nc.dims["time"]ornc.data_vars["t2m"]and gets aTypeError, or worse, a list index.Both aliases are therefore mappings.
dimsandsizesaredimension_sizes;data_varsisvariables.dimension_namesandvariable_nameskeep their names and their lists. The plan's spec called fordata_vars → variable_names; this diverges from it deliberately, and for the same reason the plan itself gavewhen it recommended making
dimsthe mapping.nc["nope"]raisesKeyErrorwherenc.get_variable("nope")raisesValueError. The mapping protocol needsKeyErrorforin,getanddict(nc)to work at all, andget_variablepredates the mapping and cannotstart raising
KeyErrorwithout breaking callers. Pinned by a test so the apparent inconsistency is not"fixed" later.
One behaviour change, in
docs/migration.mdA container now duck-types as a sequence.
__iter__and__len__flipisinstance(nc, Iterable / Sized / Container)fromFalsetoTrue.MappingandSequenceare unchanged, andbool(nc)is unaffected —Dataset.__bool__still refuses and beats__len__. The case to watch is a helper that accepts "anythingiterable": it used to reject a container outright and now receives a list of name strings.
Array coercion is deliberately not a behaviour change. NumPy would otherwise have converted a
NetCDFby looping it — which for a variable yields nothing, so
np.mean(nc["ua"])answerednanfor a cube of557,056 real values where
mainraised.NetCDF.__array__refuses, so every coercion raises exactly as itdid before, pointing at
read_array().np.array([nc], dtype=object)still boxes the dataset and is still(1,), since that request reads nothing and fabricates nothing.data_varsis not a breaking change: it has never existed onmain. It was added as a list and reshapedto the
variablesmapping within this branch, so there is nothing downstream to migrate.What
nbytesactually promisesComputed as
rows * columns * band_count * itemsize— the band axis is the whole flattened non-spatial stack,so that product is the cube and not a plane, and a cube far larger than memory can be sized. Three caveats are
on the member itself, because each makes the number mean something different:
LabeledArray, whichmaterialises its array when built. A string variable is then counted as 8 bytes per object pointer.
int16store that unpacks tofloat64costs four times the figure.
Issues
nc["t2m"],"t2m" in ncandlen(nc)all raisedata_vars,dims,sizes,attrs,coords)info(),nbytes,dtypes)Type of change
Check relevant points.
How Has This Been Tested?
Two new test modules, 243 tests, plus 60 doctests.
tests/netcdf/structure/test_container_mapping_and_aliases.py— 216 tests, markedcore. Sweeps sevenstores, one per shape the new members must survive: a plain 4-D CF cube, a container declaring dimensions its
variables do not all use, a packed 2-D store whose variables report a renamed
subset_y_...axis, a containerholding only
LabeledArrays, a grouped store withgroup/varnames, a curvilinear store that reads more namesthan it enumerates, and a single-variable store.
nc[name]andnc.variables[name]agree on every name of every store;list(nc) == variable_namesincluding order;dict(nc)round-trips;3 in ncandNone in ncareordinary
False.lengths, the single
Conventionsattribute, the CMIP store'sfloat32fields with itsint32mask,three coordinate axes. A comparison against the member an alias returns cannot catch a wrong answer.
keys()returns a copy at both levels: appending to it leaveslist(nc),len(nc),inanditems()untouched.
open_as_multi_dimensional=False, three stores):dtypes,nbytesandinfoall answer; a name the enumeration reports but GDAL will not open shows as
unknownrather than raising;dims/sizes/coordsare{}whilelen(nc)is 17; duplicate names meanlen(nc) == 12againstlen(nc.dtypes) == 9.nbytes == 0anddims == {}for a variable that plainly holds 2880bytes across four axes, because both members are container concepts.
MDArray.ReadAsArray— the call that moves bytes — notNetCDF.read_array,which none of these members calls. The names read are disjoint from
variable_namesand are alldimensions; a separate test pins the
LabeledArraycase that genuinely does read.Mapping,Sequence) and thatbool(nc)still refuses._open_variableis parametrized over all three caught refusals, with a fourth test pinning that aTypeErrorstill propagates — so a later widening toexcept Exceptionfails.tests/netcdf/parity/test_container_names_match_xarray.py— 27 tests, markedinterop.sorted(nc) == sorted(nc.to_xarray().data_vars)on the 23 stores where it holds, with the sweep tied tothe fixture directory so the exclusion list cannot grow silently.
hits two limits of xarray's data model (one flat namespace, one size per dimension name), both of which
to_xarraywarns about; the GOES and UGRID stores export an array CF classification leaves out ofvariable_names. All three remain reachable throughget_variable.Full runs on this branch:
pytest tests -m "not plot"— 12,247 passed, 89 skipped, 0 failures.pytest --doctest-modules src/pyramids/netcdf/netcdf.py— 51 passed, 9 skipped.mypy— 151 source files, no issues. (The gate was red mid-branch; thevariablesproperty wasannotated
dict[...]while returning a_LazyVariableDict.)ruff format/ruff checkat the pinned 0.15.22; no line over 120 characters.Two review rounds were run, each followed by
/testand/docstring, plus a SonarCloud sweep. Round 1 found25 issues — including a red mypy gate,
keys()handing out the list the container runs on, and six docstringclaims that were flatly untrue. Round 2 found 19 more, several of them introduced by round 1's fixes: a
variable coercing to
nan,nbytesreporting 9.7% of the true size in classic mode, and a write refusal thatclosed two of a
dictsubclass's eight mutation doors. All 44 are resolved.Checklist:
docs/reference/netcdf/public-api.mdindexes them anddocs/migration.mdcarries both behaviour changes